Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

Fix broken expectation in Safari for serializing invalid dates. #14221

Closed
wants to merge 8 commits into from

Conversation

BobChao87
Copy link
Contributor

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)

Bug fix.

What is the current behavior? (You can also link to an open issue here)

Fixes #13415. Current behaviour is that Safari throws a render error instead of correctly handling Invalid Dates (such as new Date(1/0)).

What is the new behavior (if this is a feature change)?

New behaviour is that Safari performs the same as other browsers, by serializing the invalid date or any RangeError) to 'null'.

Does this PR introduce a breaking change?

No.

Please check if the PR fulfills these requirements

Other information:

Added test to prevent regression. No docs need updating since no behaviour changed and no new features were added.

…wsers should consistently serialize to 'null'.
Fix for error thrown by Safari when an invalid date is serialized. Use 'null' always.

Closes angular#13415
Error with commit comment for Angular standards.
Fix for error thrown by Safari when an invalid date is serialized. Use 'null' always.

Closes angular#13415
Fix for error thrown by Safari when an invalid date is serialized. Use 'null' always.

Closes angular#13415
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

1 similar comment
@googlebot
Copy link

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for the commit author(s). If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.

Fix for error thrown by Safari when an invalid date is serialized. Use 'null' always.

Closes angular#13415
@BobChao87
Copy link
Contributor Author

Sorry about the spam of commits. I'm not sure what I'm doing with git. It's all new to me.

I'm not sure what to do about the CLA issue since @googlebot claims it found a CLA for me, but not for the committing party, but I am the only committing party.

Fix for error thrown by Safari when an invalid date is serialized. Use 'null' always.

Closes angular#13415
@petebacondarwin
Copy link
Contributor

The build is green now (it was just Travis being flaky)
I will find out what to do about the CLA

@petebacondarwin petebacondarwin self-assigned this Mar 19, 2016
@petebacondarwin petebacondarwin added this to the 1.5.3 milestone Mar 19, 2016
@gkalpak
Copy link
Member

gkalpak commented Mar 20, 2016

The CLA issue is probably because at some point you merged master into your branch.
You could try to rebase removing that commit (and optionally squach your other commits into one).

return JSON.stringify(obj, toJsonReplacer, pretty);
} catch (RangeError) {
// In Safari, 'Invalid Date's throw a range error. 'null' is returned in all other browsers.
return 'null';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This means that the whole Object will be serialized as "null" even if one sub-property is an invalid date.
This is inconsistent with other browsers and doesn't feel right either.

I think it is more appropriate to handle this inside the toJsonReplacer() function.

@IgorMinar
Copy link
Contributor

I was able to manually verify the CLA signature. I'll file a bug against the bot owners to support verification for users with hidden email addresses.

@petebacondarwin
Copy link
Contributor

@BobChao87 - we have sorted out the CLA problems; can you address the most recent comments then we can get this merged.

Fix for error thrown by Safari when an invalid date is serialized. Use 'null' always.

Closes angular#13415
@BobChao87
Copy link
Contributor Author

That should do it. The replacer was a good idea, I was clearly too focused on the exact example provided. Thanks @IgorMinar for the CLA fixes.

@BobChao87
Copy link
Contributor Author

Checks for the invalid date worked outside. They don't seem to work inside the toJsonReplacer because the date isn't passed in as a Date. I'll try to figure something out.

@gkalpak
Copy link
Member

gkalpak commented Mar 23, 2016

Indeed. I couldn't figure something out. Maybe this problem is not solveable (at least not without having to traverse the whole object which is obviously out of question).

Or maybe you have better luck figuring something out...

@petebacondarwin petebacondarwin modified the milestones: 1.5.3, 1.5.4 Mar 25, 2016
@Narretz
Copy link
Contributor

Narretz commented Apr 5, 2016

I think as long as the issue can't be fixed satisfactorily, we'll put this into the regular 1.5 milestone.

@Narretz Narretz modified the milestones: 1.5.x, 1.5.4 Apr 5, 2016
@petebacondarwin
Copy link
Contributor

Here is a hack that fixes this problem:

var DatetoJSON = Date.prototype.toJSON;
Date.prototype.toJSON = function() {
  try {
    return DatetoJSON.call(this);
  } catch(e) {
    if (e instanceof RangeError) {
      return null;
    }
    throw e;
  }
};

The question is how best to sniff for Safari and only do it for that browser?

@petebacondarwin
Copy link
Contributor

But since this sort of changing built in global objects is not nice, I think we cannot do this in Angular.
It is a perfectly reasonable workaround for your own apps though...

@petebacondarwin
Copy link
Contributor

I'll document this as a known issue, with this workaround over the weekend.
But I think we will have to leave this as it is and close this PR and the related issue.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Date Filter: Safari throws error if the date is an Invalid Date.
6 participants